Support wildcard cluster names in anomaly detection data source indices #1612
Conversation
bakert-96
requested review from
VijayanB,
amitgalitz,
dbwiddis,
jackiehanyang,
jmazanec15,
jngz-es,
joshpalis,
kaituo,
ohltyler,
owaiskazi19,
saratvemulapalli,
sean-zheng-amazon,
vamshin and
ylwu-amzn
as code owners
bakert-96
force-pushed
the
feature/support-cluster-wildcard
branch
from
2788eb3 to
4aa1195
Compare
… detection jobs - Updated time field and category field validation methods to query all remote indices when wildcard specified for cluster name - Added @Inject annotation for TransportService in multiple action handlers Signed-off-by: Tim Baker <[email protected]>
bakert-96
force-pushed
the
feature/support-cluster-wildcard
branch
from
4aa1195 to
34445ef
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1612 +/- ##
============================================
- Coverage 81.31% 81.07% -0.25%
+ Complexity 6144 6139 -5
============================================
Files 542 542
Lines 24998 25065 +67
Branches 2543 2557 +14
============================================
- Hits 20328 20321 -7
- Misses 3394 3461 +67
- Partials 1276 1283 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
I will get a changelog item and tests added 👌 |
|
|
||
| protected void validateTimeField(boolean indexingDryRun, ActionListener<T> listener) { | ||
| protected void validateTimeFieldInAllClusters(boolean indexingDryRun, ActionListener<T> listener) { | ||
| List<String> wildcardClusterIndices = config.getIndices().stream().filter(idx -> idx.startsWith("*:")).toList(); |
There was a problem hiding this comment.
There's duplicate code in validateTimeFieldInAllClusters and validateCategoricalFieldsInAllClusters, I suggest we extract to a method to handle some of this validation, if you want to have different validation issue types you can pass that in as variables.
| for (String remote : remoteClusters) { | ||
| List<String> remoteIndices = wildcardClusterIndices | ||
| .stream() | ||
| .map(idx -> idx.substring(2)) // remove "*:" prefix |
There was a problem hiding this comment.
nit:
here if a user passes this to indices:
"indices": [ "*:" ],
then we have empty indices being passed, we can probably filter or validate earlier here so error message makes more sense then: Exceptions: [Fail to get the index mapping of []
| import org.opensearch.timeseries.util.RestHandlerUtils; | ||
| import org.opensearch.timeseries.util.SecurityClientUtil; | ||
| import org.opensearch.transport.TransportService; | ||
| import org.opensearch.transport.*; |
There was a problem hiding this comment.
expand wildcard imports here, we try to have all imports be specific (for example:
import org.opensearch.timeseries.util.ParseUtils;
import org.opensearch.timeseries.util.RestHandlerUtils;
import org.opensearch.timeseries.util.SecurityClientUtil;
| listener | ||
| .onFailure( | ||
| new ValidationException( | ||
| "No indices specified for time field validation.", |
There was a problem hiding this comment.
Currently i know that we are inside time field validation but for this error specifically its more so that no indices are specific for the given configuration, this can be put as a ValidationIssueType.INDICES as the error here isn't necessarily for time field. Same comment can be made on similar code in checking of categorical field. if we move the code to a different method we can relook at the error messages we have.
|
Thank you for making this change! |
2 participants
Description
Updates to the validation of time field and categorical field to query all remote clusters when a wildcard cluster prefix is specified.
Related Issues
This is related to the proposal opened here #1606
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.